Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(aws_cloudwatch_logs sink): allow setting type of log class to cr… #22031

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat(aws_cloudwatch_logs sink): allow setting type of log class to cr… #22031

wants to merge 1 commit into from

Conversation

PriceHiller
Copy link
Contributor

Summary

Copied from the body of commit:

Problem: Prior to this commit it was not possible to specify the log group's class type. The method prior always created a Standard type.

Solution: Allow specifying the log group class type via a new field, group_class which takes over the create_missing_group.

deprecation: create_missing_group

Initial Issue Report: #22008

Closes #22008

This PR allows the specification of the log class to choose instead of defaulting to only the Standard log class type.

I'm not sure the approach I've taken is ideal, and I'm unsure as to the full deprecation procedure in code for the create_missing_group field.

If the approach taken here with mirroring the LogClassGroupDef is not idiomatic/poor, I'm open to suggestions (and for anything else, of course) 🙂.

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

  • By running: cargo test --no-default-features --features "sinks-console,sinks-aws_cloudwatch_logs,aws-cloudwatch-logs-integration-tests" -- --skip 'sinks::aws_cloudwatch_logs::integration_tests' 'sinks::aws_cloudwatch_logs'
  • NOTE: I was unable to run the integration tests for the cloudwatch logs. There doesn't appear to be a entry for them in the Makefile.

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

  • Please read our Vector contributor resources.
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

@PriceHiller PriceHiller requested review from a team as code owners December 13, 2024 21:12
@github-actions github-actions bot added domain: sinks Anything related to the Vector's sinks domain: external docs Anything related to Vector's external, public documentation labels Dec 13, 2024
@PriceHiller
Copy link
Contributor Author

Ah, just realized I missed a changelog entry 😅, will add in a moment.

@PriceHiller PriceHiller force-pushed the aws-cloudwatch-specify-group-class branch from 5d16838 to 7758e8f Compare December 13, 2024 21:19
@PriceHiller
Copy link
Contributor Author

Added two changelog entries in the latest force push.

@PriceHiller
feat(aws_cloudwatch_logs sink): allow setting type of log class to cr…
10fbf33 
…eate

Problem: Prior to this commit it was not possible to specify the log
group's class type. The method prior always created a Standard type.

------------------------------------------------------------

Solution: Allow specifying the log group class type via a new field,
`group_class` which takes over the `create_missing_group`.

deprecation: `create_missing_group`

Initial Issue Report: vectordotdev#22008

Closes vectordotdev#22008
@PriceHiller PriceHiller force-pushed the aws-cloudwatch-specify-group-class branch from 7758e8f to 10fbf33 Compare December 14, 2024 00:14
@PriceHiller
Copy link
Contributor Author

PriceHiller commented Dec 14, 2024 •
edited
Loading

Latest force push caught a potential regression, realized I accidentally broke support for the old behavior supported by create_missing_group.

Now if create_missing_group is enabled, we set the LogGroupClass to Some(LogGroupClass::Standard) which reflects the previous behavior.

jszwedko
jszwedko reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution @PriceHiller !

I think it isn't immediately apparent that specifying group_class will cause a group to be created. What would you think of leaving in create_missing_group without deprecation and have that control whether a group is created. group_class could then default to standard and only be used if create_missing_group is true. I realize it is a bit of redundancy, but I think the options will be more discoverable.

@PriceHiller
Copy link
Contributor Author

Thanks for this contribution @PriceHiller !

I think it isn't immediately apparent that specifying group_class will cause a group to be created. What would you think of leaving in create_missing_group without deprecation and have that control whether a group is created. group_class could then default to standard and only be used if create_missing_group is true. I realize it is a bit of redundancy, but I think the options will be more discoverable.

I think you're right, what you've proposed is probably a better approach. I can modify this PR tomorrow (hopefully) to support that form of functionality instead.

Will ping when done, for now I'll convert this PR to a draft (unless you'd prefer a new PR and me to close this).

@PriceHiller PriceHiller marked this pull request as draft December 18, 2024 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jszwedko jszwedko jszwedko left review comments

@aliciascott aliciascott aliciascott approved these changes

Assignees
No one assigned
Labels
domain: external docs Anything related to Vector's external, public documentation domain: sinks Anything related to the Vector's sinks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AWS CloudWatch Logs - create infrequent access type with create_missing_group
3 participants
@PriceHiller @jszwedko @aliciascott